Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: include catalog_visibility filter in courseware_content #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MoisesGSalas
Copy link

@MoisesGSalas MoisesGSalas commented Nov 26, 2024

The catalog_visibility filter is passed to the courseware_content index when performing a search using the legacy student dashboard view. If not included an error like follows is raised:

MeilisearchApiError. Error code: invalid_search_filter. Error message:
Attribute `catalog_visibility` is not filterable

I didn't dive to deep on this, but seems like the filter is added by the LmsSearchFilterGenerator.

The normal course search in the Learning MFE doesn't throw any errors because the additional filters are not added when the search is narrowed to a single course.

I'm not quite sure if this is the right approach considering that catalog_visibility doesn't appear in the documents for courseware_content, so the filter wouldn't really work.

I also noticed that if you configure the setting SEARCH_SKIP_INVITATION_ONLY_FILTERING=False you get Unknown value type: <class 'bool'> in both the dashboard search and the catalog search, because get_filter_rule doesn't handle the bool case, and if it did we probably would get the filter error too.

Testing instructions

On a tutor environment:

  1. Because meilisearch is not included in nightly, I cherry-picked the meilisearch commit into the nightly branch.
  2. Created a small plugin to disable the learner-dashboard MFE:
    from tutormfe.hooks import MFE_APPS
    
    @MFE_APPS.add()
    def _remove_mfes(mfes):
        mfes.pop("learner-dashboard")
        return mfes
  3. Created a test course with a single 'Text' component with a sample search term 'exampleterm'
  4. Search for 'exampleterm' on the dashboard search, it will throw 'An error occurred when searching for "exampleterm"'
  5. Use a an edx-search version with the fix and run ./manage.py lms shell -c "import search.meilisearch; search.meilisearch.create_indexes()"
  6. Repeat 4, this time the search should work.

@openedx-webhooks
Copy link

openedx-webhooks commented Nov 26, 2024

Thanks for the pull request, @MoisesGSalas!

This repository is currently maintained by @openedx/openedx-unmaintained.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.


Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 26, 2024
@MoisesGSalas MoisesGSalas marked this pull request as ready for review November 27, 2024 18:15
@pomegranited
Copy link
Contributor

Hi @MoisesGSalas , thank you for this PR, and apologies for my late reply (I've been on leave). Will review it this week.

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thank you for this fix @MoisesGSalas ! One nit to fix and we can find someone with merge rights here to merge this.

I've run out of CC time this year to dig deeper into the bugs you raised with the missing fields in this index, but feel free to create an issue on this repo if they're affecting your instances.

  • I tested this on my tutor nightly stack using the PR instructions
  • I read through the code
  • I checked for accessibility issues by using my keyboard to navigate N/A
  • Includes documentation -- one nit re that comment
  • User-facing strings are extracted for translation

search/meilisearch.py Outdated Show resolved Hide resolved
The catalog_visibility filter is passed to the courseware_content index
when performing a search using the legacy student dashboard view. If not
included an error like follows is raised:

    MeilisearchApiError. Error code: invalid_search_filter. Error message:
    Attribute `catalog_visibility` is not filterable
@MoisesGSalas MoisesGSalas force-pushed the mgs/fix-dashboard-search branch from 15f34fb to 940622a Compare December 12, 2024 21:40
@MoisesGSalas
Copy link
Author

Thanks @pomegranited, I have not experienced that error in any instance, I only noticed it while exploring the original issue with the dashboard search.

I tagged you because I saw you were the last one to work on this and for some reason I assumed you had write permissions. Can you guide me on who could be a good last reviewer?

@pomegranited
Copy link
Contributor

@MoisesGSalas

Can you guide me on who could be a good last reviewer?

Sure thing -- @Ali-Salman29 will be maintaining this repo soon. @Ali-Salman29 could you take a look at this PR for us?

@Ali-Salman29
Copy link

@pomegranited Sure, I'll look into this.

@Ali-Salman29
Copy link

I encountered the same issue with the Meilisearch query during testing. The query being sent to the Meilisearch API is as follows:

{
    'showRankingScore': True,
    'filter': [
        'course = "course-v1:Arbisoft+CS101+2024_T1"',
        'org = "Arbisoft"',
        'start_date <= 1734363448.717559 OR start_date NOT EXISTS',
        'NOT catalog_visibility = "none"'
    ],
    'offset': 0,
    'limit': 20
}

On debugging, I found that two indexes are being created in Meilisearch: course_info and courseware_content. However, the query is using the courseware_content index, which does not include the catalog_visibility field required by the filter.

Findings:

{'course_info': ['language', 'modes', 'org', 'catalog_visibility', 'enrollment_end'], 'courseware_content': ['_pk', 'course', 'org', 'start_date']}
  • The courseware_content index contains the following fields: _pk, course, org, and start_date.
  • The course_info index includes the necessary catalog_visibility field, along with other relevant fields: language, modes, org, catalog_visibility, and enrollment_end.

The query should ideally use the course_info index, as it has all the fields required by the filter. Alternatively, the courseware_content index needs to include the catalog_visibility field if it must be used for this query; as suggested in the PR.

However, it's better to first compare the indexes used for specific queries in Elasticsearch and Meilisearch. This will help us to understand if there are any discrepancies in field mappings or query behavior between the two systems.

I'll further investigate this tomorrow and share my findings here.

@Ali-Salman29
Copy link

@MoisesGSalas

{
   "properties":{
      "content":{
         "properties":{
            "display_name":{
               "type":"text",
               "fields":{
                  "keyword":{
                     "type":"keyword",
                     "ignore_above":256
                  }
               }
            },
            "html_content":{
               "type":"text",
               "fields":{
                  "keyword":{
                     "type":"keyword",
                     "ignore_above":256
                  }
               }
            }
         }
      },
      "content_groups":{
         "type":"keyword"
      },
      "content_type":{
         "type":"keyword"
      },
      "course":{
         "type":"keyword"
      },
      "course_name":{
         "type":"keyword"
      },
      "id":{
         "type":"keyword"
      },
      "location":{
         "type":"keyword"
      },
      "org":{
         "type":"keyword"
      },
      "start_date":{
         "type":"date"
      }
   }
}

Both Elasticsearch and Meilisearch use the courseware_content index for this query. There are no mappings for courseware_index in the Elasticsearch mapping, but if the filter is passed, it ignores it.

For Meilisearch, the filter list must include all the queried filters. In this case, the courseware_index is passed but there wasn't any filter for it; therefore it was throwing an error.

It's recommended to add all other missing fields content_groups, content_type, and location to the filter list so that it doesn't throw an error when these values are passed in the query.

In conclusion, the PR looks good to me. I don't have writes to merge this PR but if anyone could help it would be useful.
@pomegranited

@pomegranited
Copy link
Contributor

Thank you for your reviews @Ali-Salman29 and @MoisesGSalas! I don't have permission to merge here -- could one of you handle this, and tag a new release for us?

@Ali-Salman29
Copy link

@MoisesGSalas Can you please do it for now? My onboarding is still in progress.

@MoisesGSalas
Copy link
Author

@Ali-Salman29, sorry I can't, I don't have write permissions on this repository.

@pomegranited
Copy link
Contributor

@idegtiarov If you have some CC time available, could you help us get this merged? CC @mphilbrick211

@Ali-Salman29
Copy link

@pomegranited, I got the write access. Should I merge the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

5 participants